Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix h264 payloader stream format generation #76

Merged
merged 1 commit into from
Aug 4, 2023

Conversation

Qizot
Copy link
Member

@Qizot Qizot commented Aug 1, 2023

It happened that when only PPS parameters changed we triggered new stream format generation.

In this case, SPS values may have not been changed therefore they are absent in the currently processed buffer. This eventually results in an invalid call to hd(sps) where sps value is empty so the payloader raises.

@Qizot Qizot requested a review from mat-hek as a code owner August 1, 2023 09:33
Comment on lines +138 to +139
defp fetch_parameters_set([] = _new_ps, ps), do: ps
defp fetch_parameters_set(ps, _old_ps), do: ps
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
defp fetch_parameters_set([] = _new_ps, ps), do: ps
defp fetch_parameters_set(ps, _old_ps), do: ps
defp fetch_parameters_sets([] = _new_ps, ps), do: ps
defp fetch_parameters_sets(ps, _old_ps), do: ps

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is actually a single parameter set 😛 (either sps or pps).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it doesn't have to be, we retrieve a list here, also there can be multiple sps and multiple pps in AVCC, also there's encode_parameter_sets

@mat-hek mat-hek requested a review from varsill August 3, 2023 13:29
Copy link
Contributor

@varsill varsill left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am afraid that would still fail in case of such a stream: [pps] ... [pps] (the state.sps would be equal to nil and we would effectively try to do hd(state.sps)
I don't know if we want to handle it, either :p
[EDIT] After a brief discussion we have come to the conclusion, that it's not a problem, since the payloader is always put behind the parser, and the parser makes sure, that the [sps][pps] sequence is put at the beginning of the stream.

@varsill varsill self-requested a review August 3, 2023 14:11
@sgfn sgfn merged commit fab0354 into master Aug 4, 2023
@sgfn sgfn deleted the h264-paylodaer-missing-sps branch August 4, 2023 12:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants